-
Notifications
You must be signed in to change notification settings - Fork 910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-2716 Implements pluggable Quorum Vote #3555
Conversation
artemis-distribution/pom.xml
Outdated
@@ -183,6 +183,73 @@ | |||
<artifactId>tomcat-servlet-api</artifactId> | |||
</dependency> | |||
|
|||
<!-- for artemis-quorum-ri --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wheres the ZK alternative option dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eagle eyes :D adding them now, together with features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still dont spot them. Unresolving. I could be being blind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e208c31
to
b231f78
Compare
@@ -106,6 +110,27 @@ | |||
<include>com.sun.xml.bind:jaxb-core</include> | |||
<include>jakarta.activation:jakarta.activation-api</include> | |||
<include>jakarta.security.auth.message:jakarta.security.auth.message-api</include> | |||
<!-- quorum --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the zk deps here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
<include>org.apache.curator:curator-recipes</include> | ||
<include>org.apache.curator:curator-client</include> | ||
<include>org.apache.curator:curator-framework</include> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelandrepearce these are the ZK deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now out of office eh, still need to rebase vs master and include other deps for logging and force 3.6.1 ZK client version
b231f78
to
d2d53c6
Compare
<artifactId>classgraph</artifactId> | ||
<version>4.2.3</version> | ||
</dependency> | ||
<!-- Curator Zookeeper RI --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelandrepearce here! I still lack logging deps and config
d2d53c6
to
80e9e85
Compare
assert activeMQServer.getNodeManager().getNodeId() != null; | ||
final String liveID = activeMQServer.getNodeManager().getNodeId().toString(); | ||
final int voteRetries = policy.getVoteRetries(); | ||
final long maxAttempts = voteRetries >= 0 ? (voteRetries + 1) : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelandrepearce
@jbertram
It seems that vote-retries
can be set to be -1
(unbounded) and was used with the meaning that vote-retries
== 0 is possible, but saving any vote to happen (!).
I've decided to change the semantic to better match its name ie "retries" it seems to me (I'm not native eng, so please correct me if I'm wrong eh!) that are "additional" to the first mandatory "try".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so in retry logic.
-1 normally means to me "unlimited" e.g. never stop, keep re-trying for ever.
0 means dont do any retry, e.g. fail/error on first error, and do not re-attempt.
1 means retry once (or how ever many times, per a positive value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in regards to replication, i would typically expect.
-1 normally means wait until all members have acked.
0 means do not wait for any replica acks.
1 would wait till 1 member acked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this doc related vote-retries
:
vote-retries
This property controls how many times the backup broker retries the quorum vote in order to receive a majority result that allows the backup broker to start up.
The code on SharedNothingBackupQuorum::isLiveDown
:
final int size = quorumSize == -1 ? quorumManager.getMaxClusterSize() : quorumSize;
synchronized (voteGuard) {
for (int voteAttempts = 0; voteAttempts < voteRetries && !stopped; voteAttempts++) {
if (voteAttempts > 0) {
try {
voteGuard.wait(voteRetryWait);
} catch (InterruptedException e) {
//nothing to do here
}
}
if (!quorumManager.hasLive(targetServerID, size, quorumVoteWait, TimeUnit.SECONDS)) {
return true;
}
}
}
It doesn't seem correct related the retrying
semantic we've said above, but I would like to understand what it means.
QuorumManager::hasLive
:
- returns
true
if the majority of nodes decide that a live with NodeID ===targetServerID
is still around or ifquorumVoteWait
expires -> it will be re-attemptedvote-retries
times - returns
false
if the majority of nodes decide that a live with NodeID ===targetServerID
isn't down
Hence SharedNothingBackupQuorum::isLiveDown
is going to retry vote-retries
times until it get QuorumManager::hasLive
=== false, retrying if there is stil a live around OR there's no majority agreement in quorumVoteWait
seconds.
The new implementation is doing the same, but if vote-retries
== 0 it still try the first time before giving up, that seems more correct to me.
Sadly |
362ef2b
to
a9b7c4d
Compare
Im confused here because i thought we agreed on the call that versioned journal was needed and the assumption i took there was that would then need to be implemented before we move forwards. I dont see a need or rush to merge to master till all the bits are done from that community call. |
During the call we have agreed that I would have raised the JIRAs for the missing features to make it more robust in front of journal misalignments scenario, if compared to che classic replication. The already delivered values here are:
Users can already use what we have here getting same or better value then classic replication. In the meantime we can:
Agree about not been in hurry, but given the magnitude of this feature I believe delivering it in chunks/iteratively is the way to go (assuming each chunk to have its value). I was sure I have explained it during the call @gtully any comment? |
So i thought we had a data state issue and thus a solution was a must before we went ahead, and the point of the call in the first place to agree on that solution. As such the need for the journal versioning as a solution agreed to on the call to address that. Im very cautious about us delivering this to main and to users without an automated and safe solution to the data state issue as its asking for production issues, and people shooting themselves in the foot. Further enhancements like multi slave etc was the stuff we could implement after. But the automated data integrity was a must. |
09f5a22
to
ed32cce
Compare
I think what we have here is a very good line in the sand. the problem that a lock version solves for a computer can more easily be solved by a capable human. What we have here is a clear definition of the problem. This is a nice incremental improvement. |
And here lies the biggest issue, the human. This is what and why this was something to be solved, because too often than not, the human cannot make the right decision, starts accidentally the wrong node, so forth and so on. |
this PR is just a core part of the solution, https://issues.apache.org/jira/browse/ARTEMIS-3340 can layer over the versioning. |
Indeed, but i think we need both parts before we goto main, id be concerned about this going into main before that as then we could accidentally release just one part and people getting themselves into an issue in prod, without both parts being done, and protecting people from themselves, e.g. even myself running in prod, i would not be confident in always making the right call and indeed would want protection automatically from running the wrong one. I see no harm in keeping a branch with this stuff, until both done, if we want to have a formal feature branch in apache git, then lets make that, and then we can pull to main from there once all done. |
@michaelandrepearce I hear you. there is more work to do for sure, but it won't be in any way easier to shoot ones self in the foot with these changes. The reality is that If it is not on main, it does not exist. This feature is ripe for collaboration because it pulls in a distributed lock service, getting more users of that would be great to shape the API. The journal exclusive lock is just one use case (all be it an important one). |
Right now in testing this, yes i have been building and running it, i have started the wrong node after taking it all down, and thus ended up running the older journal, right now with leader follower (mstr, slv) setups, because of the leader (mstr) concept you always know which node should be taken down last and backup first. And on restart if you do accidentally start slave you're safe, because it wont start till master is there. If it goes to main, we're saying its ready for users, without the second part i strongly disagree i dont think it is ready for release, we can have a feature branch for collaboration, even we can produce non official binaries, but i dont think its wise to officially release it, and have users giving feedback from real life deployments where they dont fully understand what theyre buying into users != active developers, i think at this stage we're in a state that its suitable for developers yes to be running it, but they can do that from branch / overnight non release binaries. |
If it does go to main, then it should have docs written with very explicit statements (big red/orange banner like) that this is experimental not for production and work in progress. |
+100 agree @gtully |
Def want to be able to give artemis zk a base i the tree (configurable) after all many places will share a zk with other stuff or one zk for multiple brokers |
Yep, that's already implemented using the "namespace" parameter of the manager, but I would like to already separate locks from "others" (journal timestamps or any other primitives we will need in the future) |
@michaelandrepearce @gtully I used this <manager>
<properties>
<property key="connect-string" value="localhost:2181" />
<property key="namespace" value="artemis_test" />
</properties>
</manager> And using the ZK Cli cmd:
Where
That looks ok to me (name |
d547781
to
045578a
Compare
I have pulled all of the above into a new PR and added in the activation sequencing. I think we can move forward on that. |
Closing as being superseded by #3646 |
https://issues.apache.org/jira/browse/ARTEMIS-2716